fix: end the FileStreamWriter for the installer file #47

colemickens · 2023-10-23T08:52:13Z

Description

We have a failure on a repo where Error: spawn ETXTBSY was observed after #46 was merged.

Taking a closer look, we should be ~~close()-ing the WritableStream, and then~~ end()ing the FileStreamWriter.

ref:

~~WritableStream: close(): https://nodejs.org/api/webstreams.html#writablestreamclose~~
FileStreamWriter: end(): https://nodejs.org/api/stream.html#writableendchunk-encoding-callback

Checklist

Tested changes against a test repository
Added or updated relevant documentation (leave unchecked if not applicable)
(If this PR is for a release) Updated README to point to the new tag (leave unchecked if not applicable)

colemickens · 2023-10-23T09:02:12Z

Update:

pipeTo() closes the WritableStream itself (ref: https://2ality.com/2022/06/web-streams-nodejs.html ctrl-f "writablestream is closed")

Random:

this seems useful for preventing mistakes of not awaiting awaitables: https://typescript-eslint.io/rules/no-floating-promises/

Hoverbear · 2023-10-23T14:44:41Z

src/main.ts

@@ -437,6 +437,7 @@ class NixInstallerAction {
        const fileStream = fs.createWriteStream(tempfile);
        const fileStreamWeb = stream.Writable.toWeb(fileStream);
        await response.body.pipeTo(fileStreamWeb);
+        fileStream.end();


Referring to https://nodejs.org/api/fs.html#filehandlecreatewritestreamoptions, I'm not sure this syncs the file. I wonder if we also need to call...
https://nodejs.org/api/fs.html#filehandlesync

Ah, yes, and the nice solution I found is to call fs.createWriteStream with options { flush: true}, but of course that's only in node 21.

what interesting is that the code I changed wasn't calling sync explicitly, and was just waiting for the finished signal. (which I'd assume is similar to awaiting the pipeTo)

I'm going to going to really wack this with a big hammer, open the file so I get a filehandle, make sure it gets fsync'd explicitly and see how this fares.

further musing: there's a "synchronous append" mode (as) for opening, but not an equivalent for w? But there is rs+:

'rs+': Open file for reading and writing in synchronous mode. Instructs the operating system to bypass the local file system cache.

Hoverbear · 2023-10-23T18:43:02Z

Unfortunately this results in an error when running the checks:

nix-installer-action on  colemickens/fix-stream-close [$!] is 📦 v1.0.0 via  v20.8.0 via ❄️  impure (nix-shell-env) took 3s 
❯ npm run all

> nix-installer-action@1.0.0 all
> npm run build && npm run format && npm run lint && npm run package


> nix-installer-action@1.0.0 build
> tsc

src/main.ts:12:25 - error TS2307: Cannot find module 'fetch-retry' or its corresponding type declarations.

12 import fetchRetry_ from "fetch-retry";
                           ~~~~~~~~~~~~~


Found 1 error in src/main.ts:12

I realize now we don't actually note that we use the same style as https://github.com/actions/javascript-action so you need to run npm run all before every commit. :( We need to add this to the readme.

Hoverbear · 2023-10-23T18:58:45Z

#48 should fix the lack of a check!

colemickens · 2023-10-24T08:25:31Z

I agree we need #48 (and that I had a diff in dist), but the error you got was from not re-running npm install, it definitely in the package-lock.json at the most recent commit.

(edit: also, note the CI was green, so it was able to build, also pointing to it just being a lack of npm install run locally).

I'm guessing this got dropped again as part of having multiple branches in flight and rebasing. It would be good if CI checked that `npm install` and `npm run build` resulted in *zero* diffs to validate that generated/lock files match their source at time of CI.

colemickens · 2023-10-24T08:47:13Z

I rebased this on #48, hopefully we can merge it first if we want it, otherwise I'll rebase this again.

colemickens · 2023-10-24T09:18:04Z

now I'm hitting: nodejs/node#48466

colemickens force-pushed the colemickens/fix-stream-close branch 4 times, most recently from 6205da2 to 6280613 Compare October 23, 2023 08:57

colemickens requested a review from Hoverbear October 23, 2023 09:04

colemickens changed the title ~~fix: close/end the FileStreamWriter/WritableStream for the installer file~~ fix: end the FileStreamWriter for the installer file Oct 23, 2023

Hoverbear reviewed Oct 23, 2023

View reviewed changes

colemickens force-pushed the colemickens/fix-stream-close branch 4 times, most recently from 71a0da3 to d5c0386 Compare October 23, 2023 16:34

Hoverbear mentioned this pull request Oct 23, 2023

Add check for dist folder #48

Merged

3 tasks

colemickens added 2 commits October 24, 2023 10:39

eslint/tsconfig: validate no floating Promises, adjust tsconfig

4e0fccb

colemickens mentioned this pull request Oct 24, 2023

eslint/tsconfig: validate no floating Promises, adjust tsconfig #49

Merged

3 tasks

colemickens force-pushed the colemickens/fix-stream-close branch from d5c0386 to 7b08647 Compare October 24, 2023 08:46

colemickens force-pushed the colemickens/fix-stream-close branch 2 times, most recently from 36ccc35 to 2a5c00c Compare October 24, 2023 09:01

colemickens marked this pull request as draft October 24, 2023 09:03

colemickens force-pushed the colemickens/fix-stream-close branch 5 times, most recently from ba45de9 to 5c5abe9 Compare October 24, 2023 09:12

colemickens force-pushed the colemickens/fix-stream-close branch 12 times, most recently from d09e44b to 00dcbf5 Compare October 24, 2023 10:28

main.ts: installer use filehandle, make sure we fsync it before close

663467b

colemickens force-pushed the colemickens/fix-stream-close branch from 00dcbf5 to 663467b Compare October 24, 2023 15:35

retries: switch back to piping with non-web node streams

d46d5b1

colemickens force-pushed the colemickens/fix-stream-close branch from 3f99ffd to e371685 Compare October 24, 2023 15:53

retries: autoClose, but open file with O_SYNC

bb22c86

colemickens force-pushed the colemickens/fix-stream-close branch from e371685 to bb22c86 Compare October 24, 2023 16:01

colemickens marked this pull request as ready for review October 24, 2023 16:13

Hoverbear approved these changes Oct 24, 2023

View reviewed changes

Hoverbear merged commit 721f94f into main Oct 24, 2023
3 checks passed

Hoverbear deleted the colemickens/fix-stream-close branch October 24, 2023 16:31

Hoverbear mentioned this pull request Nov 2, 2023

ETXTBSY on execute_install() #51

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: end the FileStreamWriter for the installer file #47

fix: end the FileStreamWriter for the installer file #47

colemickens commented Oct 23, 2023 •

edited

Loading

colemickens commented Oct 23, 2023

Hoverbear Oct 23, 2023

colemickens Oct 23, 2023

colemickens Oct 23, 2023

colemickens Oct 23, 2023

Hoverbear commented Oct 23, 2023

Hoverbear commented Oct 23, 2023

colemickens commented Oct 24, 2023 •

edited

Loading

colemickens commented Oct 24, 2023

colemickens commented Oct 24, 2023

fix: end the FileStreamWriter for the installer file #47

fix: end the FileStreamWriter for the installer file #47

Conversation

colemickens commented Oct 23, 2023 • edited Loading

Description

Checklist

colemickens commented Oct 23, 2023

Hoverbear Oct 23, 2023

Choose a reason for hiding this comment

colemickens Oct 23, 2023

Choose a reason for hiding this comment

colemickens Oct 23, 2023

Choose a reason for hiding this comment

colemickens Oct 23, 2023

Choose a reason for hiding this comment

Hoverbear commented Oct 23, 2023

Hoverbear commented Oct 23, 2023

colemickens commented Oct 24, 2023 • edited Loading

colemickens commented Oct 24, 2023

colemickens commented Oct 24, 2023

colemickens commented Oct 23, 2023 •

edited

Loading

colemickens commented Oct 24, 2023 •

edited

Loading